Enforce NONMODIFIABLE and TRUSTED NVM policy on keystore key commit - #492
Enforce NONMODIFIABLE and TRUSTED NVM policy on keystore key commit#492yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request closes a defense-in-depth gap in the server keystore “gate” by ensuring WH_KEY_COMMIT cannot overwrite existing NVM objects that are marked NONMODIFIABLE or TRUSTED. The policy enforcement is implemented centrally in the keystore policy checker (rather than in the lower-level commit function), and is validated via a new server-side test suite.
Changes:
- Add a
WH_KS_OP_COMMITpolicy branch that consults the stored NVM object’s flags and denies overwrite whenNONMODIFIABLEorTRUSTED. - Register and add a new server test (
whTest_KeystoreCommitPolicy) covering overwrite-denial, first-commit allowance, normal commit round-trip, and the “uncached commit returns NOTFOUND” guard. - Extend the server test registry to include the new test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/wh_server_keystore.c |
Enforces NONMODIFIABLE/TRUSTED overwrite denial for commit by checking stored NVM metadata when a cache slot is being committed. |
test-refactor/server/wh_test_keystore_policy.c |
Adds targeted server-side tests validating commit policy behavior and ensuring stored bytes/flags remain unchanged on denied overwrite. |
test-refactor/wh_test_list.c |
Registers the new keystore commit policy test in the server test group. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #492
Scan targets checked: wolfhsm-core-bugs, wolfhsm-src
No new issues found in the changed files. ✅
6041613 to
0a5c6c1
Compare
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: CONDITIONAL
Findings: 6 total — 3 posted, 3 skipped
Posted findings
- [Low] Repeat/idempotent commit of a NONMODIFIABLE key now fails with WH_ERROR_ACCESS (undeclared client-visible behavior change) —
src/wh_server_keystore.c:247-263 - [Info] The TRUSTED half of the new check is only reachable via unchecked cache paths; comment does not say so —
src/wh_server_keystore.c:248-257 - [Info] Commit-overwrite does not consider NONDESTROYABLE, unlike the evict gate —
src/wh_server_keystore.c:247-263
Skipped findings
- [Low] New access-control branch has no test coverage at any level, and the stated rationale for omitting tests does not hold
- [Info] Re-committing an already-committed NONMODIFIABLE key now returns WH_ERROR_ACCESS, breaking idempotent commit retries
- [Info] Commit path now hard-depends on the optional NVM GetMetadata callback
Review generated by Skoll via Claude/Codex
0a5c6c1 to
69ca15e
Compare
|
Hello @Frauschi , |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: CONDITIONAL
Findings: 6 total — 2 posted, 4 skipped
Posted findings
- [Low] New test leaves an undeletable NONMODIFIABLE NVM object, bypassing the repo's WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS convention —
test-refactor/client-server/wh_test_crypto_keystore.c:909-911 - [Low] Commit denial returns WH_ERROR_NOTFOUND instead of WH_ERROR_ACCESS when the cache slot is gone —
src/wh_server_keystore.c:247-263
Skipped findings
- [Medium] New keystore test permanently leaks an unerasable NVM object into the shared test fixture
- [Low] wh_Client_KeyCommit is no longer idempotent for NONMODIFIABLE keys, making commit retries a hard failure
- [Info] Fail-closed unreadable-metadata branch in the new commit policy has no test coverage and returns a non-ACCESS error code
- [Info] Revoke path still writes cached bytes to NVM unchecked, contradicting the new comment on the shared switch case
Review generated by Skoll via Claude/Codex
69ca15e to
69effbc
Compare
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 10 total — 3 posted, 7 skipped
Posted findings
- [Medium] Fix guards only one of the two client-reachable commit paths; LMS/XMSS DMA keygen still commits unchecked —
src/wh_server_crypto.c:7816 - [Medium] Commit is no longer idempotent for NONMODIFIABLE keys, and the contract is undocumented —
src/wh_server_keystore.c:247 - [Medium] New NONMODIFIABLE commit regression test never compiles or runs in any buildable configuration —
test-refactor/client-server/wh_test_crypto_keystore.c:842
Skipped findings
- [Medium] New commit-denial test is compiled out in every supported configuration
- [Low] Discarded evict return lets the uncached-denial test pass for the wrong reason
- [Low] Fail-closed comment does not cover the server->nvm == NULL path
- [Low] COMMIT branch duplicates the NVM metadata lookup already performed above the switch
- [Low] Discarded evict return leaves Test 4's uncached-commit premise unverified
- [Info] Identical 32-byte key literal duplicated across the two new tests
- [Info] _KeystoreCheckPolicy now carries two divergent notions of "the key's flags"
Review generated by Skoll via Claude/Codex
69effbc to
f4dc122
Compare
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 10 total — 5 posted, 5 skipped
Posted findings
- [Medium] New CI step's CFLAGS_EXTRA override silently disables -Werror -Wall -Wextra for the only job that compiles the gated tests —
.github/workflows/build-and-test-refactor.yml:133-136 - [Medium] No test covers the TRUSTED half of the new commit mask - the actual KEK-overwrite scenario —
src/wh_server_keystore.c:261-265 - [Medium] Deny-path test could live in test-refactor/server/ and avoid the persistence gate and the new CI job entirely —
test-refactor/client-server/wh_test_crypto_keystore.c:842-931 - [Medium] Client-facing header not updated with the new non-idempotent commit contract —
wolfhsm/wh_server_keystore.h:162-179 - [Low] New WH_ERROR_NOTFOUND doc on wh_Server_KeystoreCommitKeyChecked is inaccurate, and the retry contract it documents is incomplete —
wolfhsm/wh_server_keystore.h:171-178
Skipped findings
- [Low] Gated CI job permanently consumes NVM object slots out of a budget of 30
- [Low] WH_KS_OP_REVOKE keeps the unconditional allow the COMMIT branch was added to close, under a new comment asserting it is safe
- [Info] Comment says "deny" but the code propagates the backend error code
- [Info] Header doc overstates that the verdict never comes from cache flags
- [Info] New gated test permanently consumes an NVM slot mid-run, and the TRUSTED half of the new check is untested
Review generated by Skoll via Claude/Codex
f4dc122 to
f1ed323
Compare
|
Hello @Frauschi , |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 5 total — 3 posted, 2 skipped
Posted findings
- [Medium] Revoke still launders cached flags into NVM, contradicting the new "Revocation only tightens policy" comment —
src/wh_server_keystore.c:269-271 - [Low] Suite header comment and README not updated for the two new tests —
test-refactor/client-server/wh_test_crypto_keystore.c:19-30 - [Info] New CI step covers only the ASAN configuration —
.github/workflows/build-and-test-refactor.yml:159-162
Skipped findings
- [Medium] NONMODIFIABLE deny path only runs in the new opt-in CI job
- [Low] Commit of a cached key now requires a working GetMetadata callback
Review generated by Skoll via Claude/Codex
f1ed323 to
9738bdf
Compare
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 6 total — 3 posted, 3 skipped
Posted findings
- [High] Revoke still overwrites a stored TRUSTED/NONMODIFIABLE object, contradicting the new comment and leaving f-4225 half-open —
src/wh_server_keystore.c:269-273 (comment), 1435-1440 (write) - [Medium] Uncached-commit assertion can silently degrade into a duplicate of the cached case —
test-refactor/client-server/wh_test_crypto_keystore.c:924-932 - [Medium] Fail-closed branch on unreadable NVM metadata is untested —
src/wh_server_keystore.c:252-261
Skipped findings
- [Low] README says gated suites "report SKIPPED", but the new sub-tests vanish silently
- [Low] New header contract ("caller must hold the NVM lock") is not followed by the new test added in the same PR
- [Low] 32-byte key literal and label setup duplicated verbatim between the two new client tests
Review generated by Skoll via Claude/Codex
| } | ||
| break; | ||
|
|
||
| case WH_KS_OP_REVOKE: |
There was a problem hiding this comment.
🟠 [High] Revoke still overwrites a stored TRUSTED/NONMODIFIABLE object, contradicting the new comment and leaving f-4225 half-open
🚫 BLOCK bug
The diff adds a comment on the WH_KS_OP_REVOKE case asserting _revokeKey only sets NONMODIFIABLE and clears usage flags, so revocation never grants new access. That claim is not accurate for the exact threat model the PR is fixing. wh_Server_KeystoreRevokeKey writes the cache buffer over the stored object (wh_Nvm_AddObjectWithReclaim(server->nvm, cacheMeta, cacheMeta->len, cacheBuf)), and its policy check still reads the cached flags — the same laundering the new WH_KS_OP_COMMIT branch was added to defeat. Setting up the identical pairing the PR's own new test _whTest_NvmPolicyCommitTrustedDenied builds (provision a TRUSTED KEK via wh_Nvm_AddObject, then wh_Server_KeystoreCacheKey forged bytes under the same id with TRUSTED cleared) and calling wh_Server_KeystoreRevokeKey instead of commit, I measured on this branch: revoke rc=0, stored byte 0 became 0xFF (the forged bytes, was 0x40), stored flags became 0x0001 (NONMODIFIABLE — TRUSTED was dropped). So a client can still destroy and replace a trusted KEK's stored material through WH_KEY_REVOKE, and the resulting object is NONMODIFIABLE so wh_Nvm_DestroyObjectsChecked will not let it be repaired. The cache-side reachability is the same one the PR cites for commit: _HandleKeyUnwrapAndCacheRequest calls the unchecked wh_Server_KeystoreCacheKey with a blob-supplied id (src/wh_server_keystore.c:2452), and WH_KEY_REVOKE (src/wh_server_keystore.c:3281) is client-reachable. The revoke code itself is pre-existing, but the diff introduces a comment asserting the property holds, and the PR is filed as closing f-4225 for exactly this overwrite class.
Suggestion:
| case WH_KS_OP_REVOKE: | |
| case WH_KS_OP_REVOKE: | |
| /* Revoke rewrites the stored object from the cache slot, so it is | |
| * an overwrite like commit: consult the stored flags, not the | |
| * cached ones, or an unchecked cache path can launder them. */ | |
| if (!foundInNvm && (server->nvm != NULL)) { | |
| ret = wh_Nvm_GetMetadata(server->nvm, keyId, &nvmMeta); | |
| if (ret == WH_ERROR_OK) { | |
| foundInNvm = 1; | |
| } | |
| else if (ret != WH_ERROR_NOTFOUND) { | |
| return ret; | |
| } | |
| } | |
| if (foundInNvm && (nvmMeta.flags & WH_NVM_FLAGS_TRUSTED)) { | |
| return WH_ERROR_ACCESS; | |
| } | |
| break; |
Recommendation: Gate WH_KS_OP_REVOKE on the stored object's WH_NVM_FLAGS_TRUSTED the same way commit now is (revoking a TRUSTED KEK is never a legitimate client operation). NONMODIFIABLE must stay permitted for revoke so an already-revoked key can be re-revoked, but TRUSTED can be denied without changing any normal flow. At minimum, correct the new comment and state the residual gap explicitly, and consider whether f-4225 should stay open. A server-side regression test mirroring _whTest_NvmPolicyCommitTrustedDenied but calling wh_Server_KeystoreRevokeKey reproduces this in a few lines.
|
|
||
| /* The key cannot be erased: wh_Nvm_DestroyObjectsChecked refuses a | ||
| * NONMODIFIABLE object, so only the cache slot is reclaimed here. */ | ||
| (void)wh_Client_KeyEvict(ctx, keyId); |
There was a problem hiding this comment.
🟡 [Medium] Uncached-commit assertion can silently degrade into a duplicate of the cached case
💡 SUGGEST test
Test 4 claims to prove the denial is independent of cache residency: the denial does not depend on cache residency. With no slot left, the stored flags still decide. But the eviction that is supposed to create the no-slot state discards its return with (void)wh_Client_KeyEvict(ctx, keyId);. The preceding wh_Client_KeyExport re-populates the cache via wh_Server_KeystoreReadKey, which only marks the freshened slot committed when wh_Server_KeystoreCacheKey succeeds (src/wh_server_keystore.c:1141). If that ever stops happening, WH_KS_OP_EVICT denies the evict for a NONMODIFIABLE key, the slot stays cached, and Test 4 still passes — but only as a re-run of Test 2, silently losing the coverage the comment claims. Test 3's evict is checked; this one should be too.
Suggestion:
| (void)wh_Client_KeyEvict(ctx, keyId); | |
| ret = wh_Client_KeyEvict(ctx, keyId); | |
| if (ret != 0) { | |
| WH_ERROR_PRINT("Failed to evict before uncached commit check: %d\n", | |
| ret); | |
| return ret; | |
| } |
Recommendation: Check the eviction return so the test fails loudly if the slot is still resident, rather than asserting a property it did not actually set up.
| /* Stored flags decide, not cached, so an unchecked cache path | ||
| * cannot launder them; same pair as wh_Nvm_AddObjectChecked. | ||
| * Fetched here too, so the verdict ignores cache residency. */ | ||
| if (!foundInNvm && (server->nvm != NULL)) { |
There was a problem hiding this comment.
🟡 [Medium] Fail-closed branch on unreadable NVM metadata is untested
💡 SUGGEST test
The new WH_KS_OP_COMMIT branch has three outcomes: no stored object (allow), stored object with the flags (deny), and metadata unreadable (return ret, fail closed). The PR's tests cover the first two — _whTest_ModifiableRecommit / _whTest_NonModifiableCommit client-side and _whTest_NvmPolicyCommitTrustedDenied server-side — but nothing exercises the ret != WH_ERROR_NOTFOUND path. That branch is the one that decides a backend error denies rather than silently permits the write, which is the security-relevant default. The repo already has a fault-injection backend (test-refactor/posix/wh_test_flash_fault_inject.c), so this is testable without new infrastructure.
Recommendation: Add a server-side case that makes wh_Nvm_GetMetadata return a non-NOTFOUND error for a cached key and asserts the commit is refused rather than allowed, using the existing flash fault-injection harness.
Problem
wh_Server_KeystoreCommitKeywrites the cached key to NVM through theunchecked
wh_Nvm_AddObjectWithReclaim, so nothing consulted the storedobject's flags. A client could overwrite an existing
NONMODIFIABLEorTRUSTEDNVM object — including a trusted KEK — by caching a key under its idand committing. Closes f-4225.
Fix (
src/wh_server_keystore.c)Added a
WH_KS_OP_COMMITbranch to_KeystoreCheckPolicythat reads thestored object's metadata and denies the overwrite:
(unwrap-and-cache, SHE) cannot launder the policy by populating a slot whose
flags do not reflect the stored object.
NONMODIFIABLE | TRUSTED— the same pair aswh_Nvm_AddObjectChecked,the correct model for an add-shaped operation.
NONDESTROYABLEgatesdestroy, not overwrite.
nvmMetawhetheror not the cache slot survived, so a denied commit always reports
WH_ERROR_ACCESSinstead of theWH_ERROR_NOTFOUNDraised by a missing slot.cannot be policed, so the commit is denied rather than blind.
Client-visible behavior change: re-committing a cached key whose stored
object is
NONMODIFIABLEnow returnsWH_ERROR_ACCESSwhere a byte-identicalrewrite previously returned
WH_ERROR_OK. This makes commit consistent withevery other immutable-write path —
_NvmCheckPolicy(WH_NVM_OP_ADD) alreadyrefuses a no-op rewrite, and commit was the only exception. The contract is now
documented on
wh_Server_KeystoreCommitKeyChecked(
wolfhsm/wh_server_keystore.h): a client retrying after a lost response musttreat
WH_ERROR_ACCESSas "already committed". Revoke is unaffected; it keepsits own "already revoked and committed" short-circuit.
Tests
Added to
test-refactor/client-server/wh_test_crypto_keystore.c, driven onlyby
wh_Client_*:_whTest_NonModifiableCommit— first commit succeeds, repeat commit returnsWH_ERROR_ACCESS, the denial still holds once the slot is evicted, and thestored bytes and label survive the denial. Gated behind
WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS, since a committedNONMODIFIABLEobject cannot be erased._whTest_ModifiableRecommit— ungated; a key without the flag still commitsrepeatedly.
Making that gate runnable.
test-refactorcould not build with the macrodefined at all:
wh_test_crypto_keypolicy.ccallsWH_CLIENT_DEVID(client)inside a helper that takes no client context (pre-existing on
main, so bothgated suites were dead code). Threading the context into
whTest_RevocationTryAESEncryptfixes it, and a new trailing step in.github/workflows/build-and-test-refactor.ymlbuilds and runs with the macrodefined. That revives this PR's deny-path test and the keypolicy AES-CBC
revocation test, dormant since
606866e.Verification
DMA=1 ASAN=1-std=c90 -Werror -Wall -Wextra,ASan clean. Legacy
test/suite exits 0.WH_KS_OP_COMMITbranch reverted, the gatedrun fails —
Non-modifiable key was re-committed unexpectedly: 0.